Skip to content

Comments

Fix ContentAlign and AlignItems combination cause wrong behaviour#1013

Closed
woehrl01 wants to merge 3 commits intofacebook:mainfrom
woehrl01:fix-wrap-and-align
Closed

Fix ContentAlign and AlignItems combination cause wrong behaviour#1013
woehrl01 wants to merge 3 commits intofacebook:mainfrom
woehrl01:fix-wrap-and-align

Conversation

@woehrl01
Copy link
Contributor

@woehrl01 woehrl01 commented Jun 9, 2020

This PR fixes #1008, where the combination of ContentAlign and AlignItems cause wrong behavior.

Reason for that was that the cross dimensional lead has been mistakenly uses in the calculation of the item position during the AlignItems step (for non-stretch values).

Added the specific tests to prevent future regressions.

facebook-github-bot pushed a commit to facebook/litho that referenced this pull request Oct 18, 2023
Summary:
X-link: facebook/react-native#41019

### Changes made
- Regenerated tests (as some aspect ratio tests seem to be out of date compared to the fixtures)
- Added SpaceEvenly variant to the "Align" enums (via enums.py)
- Implemented `align-content: space-evenly` alignment in CalculateLayout.cpp
- Added generated tests `align-content: space-evenly`
- Updated NumericBitfield test to account for the fact that the Align enum now requires more bits (this bit could do with being reviewed as I am not 100% certain that it's valid to just update the test like this).

### Changes not made
- Any attempt to improve the spec-compliance of content alignment in general (e.g. I think facebook/yoga#1013 probably still needs to happen)

X-link: facebook/yoga#1422

Reviewed By: yungsters

Differential Revision: D50305438

Pulled By: NickGerleman

fbshipit-source-id: ef9f6f14220a0db066bc30db8dd690a4a82a0b00
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Oct 18, 2023
Summary:
Pull Request resolved: #41019

### Changes made
- Regenerated tests (as some aspect ratio tests seem to be out of date compared to the fixtures)
- Added SpaceEvenly variant to the "Align" enums (via enums.py)
- Implemented `align-content: space-evenly` alignment in CalculateLayout.cpp
- Added generated tests `align-content: space-evenly`
- Updated NumericBitfield test to account for the fact that the Align enum now requires more bits (this bit could do with being reviewed as I am not 100% certain that it's valid to just update the test like this).

### Changes not made
- Any attempt to improve the spec-compliance of content alignment in general (e.g. I think facebook/yoga#1013 probably still needs to happen)

X-link: facebook/yoga#1422

Reviewed By: yungsters

Differential Revision: D50305438

Pulled By: NickGerleman

fbshipit-source-id: ef9f6f14220a0db066bc30db8dd690a4a82a0b00
facebook-github-bot pushed a commit that referenced this pull request Oct 18, 2023
Summary:
X-link: facebook/react-native#41019

### Changes made
- Regenerated tests (as some aspect ratio tests seem to be out of date compared to the fixtures)
- Added SpaceEvenly variant to the "Align" enums (via enums.py)
- Implemented `align-content: space-evenly` alignment in CalculateLayout.cpp
- Added generated tests `align-content: space-evenly`
- Updated NumericBitfield test to account for the fact that the Align enum now requires more bits (this bit could do with being reviewed as I am not 100% certain that it's valid to just update the test like this).

### Changes not made
- Any attempt to improve the spec-compliance of content alignment in general (e.g. I think #1013 probably still needs to happen)

Pull Request resolved: #1422

Reviewed By: yungsters

Differential Revision: D50305438

Pulled By: NickGerleman

fbshipit-source-id: ef9f6f14220a0db066bc30db8dd690a4a82a0b00
NickGerleman added a commit to NickGerleman/yoga that referenced this pull request Dec 12, 2023
Summary:
Fixes facebook#1300
Fixes facebook#1008

This fixes a smattering of issues related to both sizing and aligment of multi-line-containers:

1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container.
2. Per-line leads were sometimes incorrectly contributing to alignment within the line
3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint.

Need to test how breaking this is, to see if it might need to go behind an errata.

See related PRs:
1. facebook#1491
2. facebook#1493
3. facebook#1013

Changelog:
[General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers

Differential Revision: D52087013
NickGerleman added a commit to NickGerleman/yoga that referenced this pull request Dec 12, 2023
…1513)

Summary:

X-link: facebook/react-native#41916

Fixes facebook#1300
Fixes facebook#1008

This fixes a smattering of issues related to both sizing and aligment of multi-line-containers:

1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container.
2. Per-line leads were sometimes incorrectly contributing to alignment within the line
3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint.

Need to test how breaking this is, to see if it might need to go behind an errata.

See related PRs:
1. facebook#1491
2. facebook#1493
3. facebook#1013

Changelog:
[General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers

Differential Revision: D52087013
NickGerleman added a commit to NickGerleman/yoga that referenced this pull request Dec 12, 2023
…1513)

Summary:

X-link: facebook/react-native#41916

Fixes facebook#1300
Fixes facebook#1008

This fixes a smattering of issues related to both sizing and aligment of multi-line-containers:

1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container.
2. Per-line leads were sometimes incorrectly contributing to alignment within the line
3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint.

Need to test how breaking this is, to see if it might need to go behind an errata.

See related PRs:
1. facebook#1491
2. facebook#1493
3. facebook#1013

Changelog:
[General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers

Differential Revision: D52087013
NickGerleman added a commit to NickGerleman/yoga that referenced this pull request Dec 16, 2023
…1513)

Summary:

X-link: facebook/react-native#41916

Fixes facebook#1300
Fixes facebook#1008

This fixes a smattering of issues related to both sizing and aligment of multi-line-containers:

1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container.
2. Per-line leads were sometimes incorrectly contributing to alignment within the line
3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint.

Need to test how breaking this is, to see if it might need to go behind an errata.

See related PRs:
1. facebook#1491
2. facebook#1493
3. facebook#1013

Changelog:
[General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers

Reviewed By: joevilches

Differential Revision: D52087013
NickGerleman added a commit to NickGerleman/yoga that referenced this pull request Dec 16, 2023
…1513)

Summary:

X-link: facebook/react-native#41916

Fixes facebook#1300
Fixes facebook#1008

This fixes a smattering of issues related to both sizing and aligment of multi-line-containers:

1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container.
2. Per-line leads were sometimes incorrectly contributing to alignment within the line
3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint.

Need to test how breaking this is, to see if it might need to go behind an errata.

See related PRs:
1. facebook#1491
2. facebook#1493
3. facebook#1013

Changelog:
[General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers

Reviewed By: joevilches

Differential Revision: D52087013
NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Dec 16, 2023
…41916)

Summary:
X-link: facebook/yoga#1513


Fixes facebook/yoga#1300
Fixes facebook/yoga#1008

This fixes a smattering of issues related to both sizing and aligment of multi-line-containers:

1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container.
2. Per-line leads were sometimes incorrectly contributing to alignment within the line
3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint.

Need to test how breaking this is, to see if it might need to go behind an errata.

See related PRs:
1. facebook/yoga#1491
2. facebook/yoga#1493
3. facebook/yoga#1013

Changelog:
[General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers

Reviewed By: joevilches

Differential Revision: D52087013
facebook-github-bot pushed a commit to facebook/litho that referenced this pull request Dec 16, 2023
Summary:
X-link: facebook/yoga#1513

X-link: facebook/react-native#41916

Fixes facebook/yoga#1300
Fixes facebook/yoga#1008

This fixes a smattering of issues related to both sizing and aligment of multi-line-containers:

1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container.
2. Per-line leads were sometimes incorrectly contributing to alignment within the line
3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint.

Need to test how breaking this is, to see if it might need to go behind an errata.

See related PRs:
1. facebook/yoga#1491
2. facebook/yoga#1493
3. facebook/yoga#1013

Changelog:
[General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers

Reviewed By: joevilches

Differential Revision: D52087013

fbshipit-source-id: 8d95ad17e58c1fec1cceab9756413d0b3bd4cd8f
facebook-github-bot pushed a commit that referenced this pull request Dec 16, 2023
Summary:
Pull Request resolved: #1513

X-link: facebook/react-native#41916

Fixes #1300
Fixes #1008

This fixes a smattering of issues related to both sizing and aligment of multi-line-containers:

1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container.
2. Per-line leads were sometimes incorrectly contributing to alignment within the line
3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint.

Need to test how breaking this is, to see if it might need to go behind an errata.

See related PRs:
1. #1491
2. #1493
3. #1013

Changelog:
[General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers

Reviewed By: joevilches

Differential Revision: D52087013

fbshipit-source-id: 8d95ad17e58c1fec1cceab9756413d0b3bd4cd8f
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Dec 16, 2023
Summary:
X-link: facebook/yoga#1513

Pull Request resolved: #41916

Fixes facebook/yoga#1300
Fixes facebook/yoga#1008

This fixes a smattering of issues related to both sizing and aligment of multi-line-containers:

1. We were previously incorrectly bounding the size of each flex line to the min/max of the entire container.
2. Per-line leads were sometimes incorrectly contributing to alignment within the line
3. The cross dim size used for multi-line alignment is not correct, or correctly clamped. If the available size comes from a max constraint, that was incorrectly used instead of a definite size, or size of content. Leads were entirely skipped for min constraint.

Need to test how breaking this is, to see if it might need to go behind an errata.

See related PRs:
1. facebook/yoga#1491
2. facebook/yoga#1493
3. facebook/yoga#1013

Changelog:
[General][Fixed] - Fix Yoga sizing and alignment issues with multi-line containers

Reviewed By: joevilches

Differential Revision: D52087013

fbshipit-source-id: 8d95ad17e58c1fec1cceab9756413d0b3bd4cd8f
@NickGerleman
Copy link
Contributor

I incorporated a fix for this into 464d166

tungdo194 pushed a commit to tungdo194/rn-test that referenced this pull request Apr 28, 2024
Summary:

### Changes made
- Regenerated tests (as some aspect ratio tests seem to be out of date compared to the fixtures)
- Added SpaceEvenly variant to the "Align" enums (via enums.py)
- Implemented `align-content: space-evenly` alignment in CalculateLayout.cpp
- Added generated tests `align-content: space-evenly`
- Updated NumericBitfield test to account for the fact that the Align enum now requires more bits (this bit could do with being reviewed as I am not 100% certain that it's valid to just update the test like this).

### Changes not made
- Any attempt to improve the spec-compliance of content alignment in general (e.g. I think facebook/yoga#1013 probably still needs to happen)

X-link: facebook/yoga#1422

Reviewed By: yungsters

Differential Revision: D50305438

Pulled By: NickGerleman
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ContentAlign and AlignItems combination cause wrong behaviour

3 participants